Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: in-line versioning + deprecation for multi-semaphore #13645

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

agilgur5
Copy link
Member

@agilgur5 agilgur5 commented Sep 22, 2024

Addresses #13644, #13358 (comment) etc:

Since people will look at the examples and not know that they have future syntax etc

Indeed it looks like this is happening almost as soon as this was merged 😕 #13644 plus two Slack threads. The issue and former Slack thread are also from a long time user, i.e. not a newbie.

And ofc this contributor thread and previous Contributor Meeting with Alan and I butting heads mostly over in-line version specifiers 😅

Motivation

Mitigate users not realizing they're on the latest version of the docs and attempting to use features that don't exist yet, getting errors, and then filing issues or support requests about it.
To some extent, it is inevitable, but in-line versioning is a mitigation in an effort to help users self-service.

Modifications

  • remove warning about using mutexes and semaphores together as that is possible in 3.6 after feat: multiple mutexes and semaphores #13358 -- the warning should only be in earlier versions

  • use snippet embeds to directly use examples in the docs without duplication

  • add in-line comments with version specifiers on mutexes and semaphores syntax

  • add deprecation comments with version specifiers on mutex and semaphore syntax

  • add alternative syntax in comments, as suggested in feat: multiple mutexes and semaphores #13358 (comment)

    • this should make it very hard for users to get wrong, which is the intent
  • remove "Legacy" section as this does not match conventions

    • we conventionally don't explicitly describe prior state in the docs and leave that to the older versions of the docs (Versioned documentation #11390)
      • (if we did, we could have "legacy" sections ad infinitum. In the Slack thread, this is also 3., that we do not do and is one of the purposes of versioned docs)
    • users also don't read separate sections like this, they get an error and file an issue before they even reach this section, literally per semaphores error: cannot get LockName for a Sync of Unknown type #13644 et al
    • the in-line comments cover this with more direct examples and as you're reading the example instead of after. this follows "show, don't tell" and style guide on directness.
      • the in-line is also more implicit of a reference to older syntax as opposed to an entire explicit section on it
    • we also conventionally don't describe bugs in the docs; combining mutex with semaphore and it not erroring out but silently ignoring the mutex is a confusing bug.
      • (if we did describe bugs, we could have those ad infinitum in the docs as well)

Verification

make docs passes.

Here's a screenshot of the example snippet embeds:

Screenshot 2024-09-22 at 1 06 26 PM

Future Work

  1. Use embeds for examples in more places in the docs

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
…d was not read either

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
…al IMO

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5 agilgur5 added area/docs Incorrect, missing, or mistakes in docs area/mutex-semaphore labels Sep 22, 2024
@@ -24,10 +24,6 @@ data:
template: "2" # Two instances of template can run at a given time in particular namespace
```

!!! Warning
Each synchronization block may only refer to either a semaphore or a mutex.
If you specify both only the semaphore will be locked.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer relevant in 3.6

...
```

Specifying both would not work in <3.6, only the semaphore would be used.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We just need to fix this bug per #13358 (comment) and #11859 -- i.e. give a validation error instead of silently ignoring it in 3.5 and prior versions

Copy link
Member Author

@agilgur5 agilgur5 Sep 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed with a validation error in #13669


The single `mutex` and `semaphore` syntax still works in version 3.6 but is considered deprecated.
Both the `mutex` and the `semaphore` will be taken in version 3.6 with this syntax.
This syntax can be mixed with `mutexes` and `semaphores`, all locks will be required.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we want to describe this, this would be better described with another example

name: welcome
# mutexes: # v3.6 and after
# - name: welcome
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm inclined to just move these to internal tests and otherwise remove the legacy examples entirely from the user-facing examples/ dir

and flipped syntax + in-line comments here is a stopgap, especially as the in-line deprecation is hard to understand without the future syntax right next to it (and users are likely to miss -legacy name too)

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- [`synchronization-mutex-wf-level-legacy.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/synchronization-mutex-wf-level-legacy.yaml)

- [`synchronization-mutex-wf-level.yaml`](https://github.com/argoproj/argo-workflows/blob/main/examples/synchronization-mutex-wf-level.yaml)
Copy link
Member Author

@agilgur5 agilgur5 Sep 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is suboptimal as the field checker automation is plain text search and so does not recognize that it's in a comment, but nbd in context as it does show it and say it's deprecated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docs Incorrect, missing, or mistakes in docs area/mutex-semaphore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant